Improve `HalfEdgeTopology(::Vector{<:Connectivity})` correctness #1194

halleysfifthinc · 2025-05-13T19:36:43Z

This PR reveals/fixes a couple of bugs in the HalfEdgeTopology constructor.

Dict's are unordered, so constructing the halves::Vector{Tuple{HalfEdge,HalfEdge}} by iterating through half4pair pairs produces a roughly random order (by element) for halves.

Meshes.jl/src/topologies/halfedge.jl

Line 220 in 3a3d0e3

for ((u, v), he) in half4pair

This has downstream affects on e.g. edge indices and the starting vertex index from loop. (Cause of most/all? test failures.)
Incorrect orientation consistency handling in the presence of incorrectly- or unsorted elements. A couple of things to note:
- Treating incorrectly- or unsorted elements as a "no warranty" situation is perfectly fine, but the current/master state of the function can lead to non-closed edge loops, such that the loop function never terminates and allocates memory until Julia is OOM killed¹.
  - ```
  # without this redefinition, `loop` will eat all your memory
  function Meshes.loop(e::HalfEdge)
  n = e.next
  v = [e.head]
  cnt = 1
  while n != e
      if cnt < 3
          push!(v, n.head)
          n = n.next
          cnt += 1
      else
          throw(error("too many edges!"))
      end
  end
  v
  end
  
  e = connect.([(1,2,3), (1,3,4), (2,5,3), (5,4,6), (3,5,4)], Triangle)
  t = HalfEdgeTopology(e; sort=false)
  Meshes.loop(half4elem(t, 1)) # works
  Meshes.loop(half4elem(t, 2)) # errors
```
- Minor nit: The orientation consistency variable name (CCW) was misleading, as actual element orientation isn't checked (and cannot be without actual vertices afaict?). Also, when there are multiple unconnected components (which I believe is still valid under a 2-manifold?) the orientation consistency checks don't/can't cross the disconnection, leading to further potential mismatches between the name CCW and reality. I renamed it to make this clearer.

The cause of most of the current test failures are due to changing to the halves construction to be element ordered. I believe this is more correct, and that any stable/consistent edge indices with the previous version are because the hashing algorithm in Base has not been changed between Julia versions (that will no longer be true after 1.13 with JuliaLang/julia#57509).

Another cause of test failures is a change to the reverse index iteration, which now ensures that the first index returned by loop (which affects several downstream dependent functions) is either:

The original first index of the element (regardless of whether the original element had an inconsistent orientation)
The (head) index of the first observed edge of the element

With regard to sorting and orientation consistency, the PR is correct (no infinite memory use) even for incorrectly- or unsorted elements when called with sort=false, but the edge indices will/may change between sort=false and sort=true.

If you agree with this PR's general direction. I will update the tests and share benchmarks.

In my code, I am redefining loop similar to the given example, but using a ScopedValue so that the maximum number of edges can be changed from outside the function if needed. I'm happy to share/submit that if you are interested, but I realize that such a solution might be too limiting generally to be added to the library. ↩

The code/algorithm doesn't actually check for counter-clockwise-ness, it only knows whether something has an inconsistent orientation with previously added elements (ultimately going back to whichever element was added first--whose orientation was never checked)

juliohm

Thank you @halleysfifthinc ! Left a few minor comments. I can review again after we get green tests.

test/topologies.jl

src/topologies/halfedge.jl

Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>

codecov · 2025-05-14T18:10:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.39%. Comparing base (3a3d0e3) to head (8ff06b6).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1194      +/-   ##
==========================================
+ Coverage   88.31%   88.39%   +0.07%     
==========================================
  Files         196      196              
  Lines        6094     6133      +39     
==========================================
+ Hits         5382     5421      +39     
  Misses        712      712

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

test/toporelations.jl

test/topologies.jl

halleysfifthinc · 2025-05-14T18:36:30Z

Benchmarking procedure same as previously described, using sort=false to limit extraneous testing time/allocations:

Quads only:

BenchmarkTools.TrialJudgement: 
  time:   -66.93% => improvement (5.00% tolerance)
  memory: -46.14% => improvement (1.00% tolerance)

Quads and tris:

BenchmarkTools.TrialJudgement: 
  time:   -66.81% => improvement (5.00% tolerance)
  memory: -43.88% => improvement (1.00% tolerance)

juliohm · 2025-05-19T13:02:00Z

Thank you for the updates @halleysfifthinc. I am planning to review a couple of PRs in CoordRefSystems.jl, Meshes.jl and GeoIO.jl this week.

juliohm · 2025-05-20T00:02:19Z

@halleysfifthinc the tests were failing locally for me, falling in the assertion you introduced. I did some cleanup to facilitate the review moving forward.

src/topologies/halfedge.jl

halleysfifthinc · 2025-05-20T17:08:20Z

the tests were failing locally for me

Before or after the readability changes in dab47eb? Tests are consistently passing for me before that commit, and also consistently pass after I sidestep the base Julia bug. 🤞 nothing turns up in this CI run

juliohm · 2025-05-20T17:10:01Z

Before or after the readability changes in dab47eb?

Almost sure it was before the changes. I did run the tests locally, then made the changes.

halleysfifthinc · 2025-05-20T17:32:25Z

Hmmm. Yesterday, I did see a test failure once before the readability commit, but I'm not able to replicate that today. I wonder if it was user error on my part (e.g. I wasn't actually on the right commit, etc).

I have been running one of the previously failing tests in an infinite loop for >20mins now and have not triggered any more errors. (I also have been running with these patches [with the const NULLEDGE version] for several days without experiencing any issues either.)

juliohm · 2025-05-20T17:51:19Z

I confirm that all tests pass locally 💯

Will review the test changes one by one now, before assessing the performance improvements 👍🏽

src/topologies/halfedge.jl

test/topologies.jl

juliohm · 2025-05-20T18:38:09Z

All test changes are perfect 💯 I will benchmark the PR against the master branch now...

juliohm · 2025-05-20T19:41:00Z

I confirm a speedup of approximately 2x when sort=false 🚀

Copilot

Pull Request Overview

This PR fixes nondeterministic ordering and orientation bugs in the HalfEdgeTopology constructor and updates related tests.

Rewrites half-edge pairing logic to produce deterministic, element‐ordered halves and correctly handle element orientation.
Introduces helper functions (anyhalf, anyhalfclaimed) and refactors adjsort placement.
Adjusts test expectations throughout test/toporelations.jl and test/topologies.jl to match the new ordering/orientation behavior.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
test/toporelations.jl	Updated expected boundary/coboundary tuples to reflect new half-edge order.
test/topologies.jl	Added invariants for half-edge `prev`/`next`, updated element ordering tests.
src/topologies/halfedge.jl	Overhauled `HalfEdgeTopology` constructor, added helper functions, removed old inline `adjsort`.

Comments suppressed due to low confidence (1)

src/topologies/halfedge.jl:373

[nitpick] The name anyhalf is ambiguous. Consider renaming to something like has_shared_halfedge and anyhalfclaimed to has_claimed_halfedge for clarity.

function anyhalf(inds, half4pair)

src/topologies/halfedge.jl

juliohm · 2025-05-20T19:47:05Z

@JoshuaLampert do you have any additional comment before we merge this? I know it is a very tricky algorithm to follow, so don't feel any pressure to review in detail. Any bird's-eye review would be nice given the sensitivity of these changes 🙂

juliohm · 2025-05-20T19:53:18Z

The test that is failing consistently in MacOS is unrelated to the changes in this PR. We need to increase the threshold in a separate PR later.

JoshuaLampert

Thanks for the contribution, @halleysfifthinc! I had a quick look and didn't find any issue standing out. But, as you suggested @juliohm, I didn't look at the algorithm, but had only an abstract view. Generally, as someone who only has a very rough idea of what the code is supposed to do, I find the code quite hard to read, but I guess there is not much to do against this. I was wondering whether this counts as breaking change because, IIUC, it changes the ordering. Or was the ordering nothing to rely on before anyway and therefore this change can be done in a non-breaking way?

src/topologies/halfedge.jl

juliohm · 2025-05-20T20:42:46Z

I was wondering whether this counts as breaking change because, IIUC, it changes the ordering. Or was the ordering nothing to rely on before anyway and therefore this change can be done in a non-breaking way?

The ordering of vertices in HalfEdgeTopology is arbitrary and hidden from end users through our topological relations Adjacency, Boundary and CoBoundary. Our idea with the current design is that users can write algorithms in terms of topological relations, which are agnostic to the topological data structure underneath.

Originally I thought of releasing this as a patch release to upstream the performance improvements throughout the ecosystem, but if you guys feel that this has the potential to break code, we can go with a minor release.

juliohm · 2025-05-20T20:50:12Z

Thank you all for the valuable contributions! Merging another important fix and speedup 🚀 👏🏽

halleysfifthinc · 2025-05-20T20:57:11Z

if you guys feel that this has the potential to break code, we can go with a minor release

I would be consider this a non-breaking bugfix, as the previous indices were indisputably (IMO) incorrect (albeit, consistently so). Any code relying on the previous orderings is potentially also incorrect, and therefore implicitly already broken.

juliohm · 2025-05-20T20:59:56Z

Very good point. This PR also fixes a bug so it would be a good idea to propagate the fix. Em ter., 20 de mai. de 2025, 17:57, Allen Hill ***@***.***> escreveu:

…

*halleysfifthinc* left a comment (JuliaGeometry/Meshes.jl#1194) <#1194 (comment)> if you guys feel that this has the potential to break code, we can go with a minor release I would be consider this a non-breaking bugfix, as the previous indices were indisputably (IMO) incorrect (albeit, consistently so). Any code relying on the previous orderings is potentially also incorrect, and therefore implicitly already broken. — Reply to this email directly, view it on GitHub <#1194 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZQW3MYQJI75WQ4ZELXJ4L27OJL3AVCNFSM6AAAAAB5BPSIEGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQOJVHAYTIMBXGI> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

JoshuaLampert · 2025-05-20T21:12:30Z

It's fine with me to not consider it as breaking.

halleysfifthinc added 5 commits May 13, 2025 12:39

Expand test_halfedge function and add new test

b68dd64

Refactor `HalfEdgeTopology(::Vector{Connectivity})

6e60238

collect all indices at the beginning

836a0db

Keep first index the same when reversing orientation

16aa7c4

halleysfifthinc force-pushed the halfedge-topo-constructor branch from 2a6d072 to 16aa7c4 Compare May 13, 2025 19:39

juliohm requested changes May 14, 2025

View reviewed changes

halleysfifthinc and others added 3 commits May 14, 2025 09:13

Update test/topologies.jl

27deeb8

Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>

Update test/topologies.jl

5a0c047

Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>

Update src/topologies/halfedge.jl

f9f7418

Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>

halleysfifthinc commented May 14, 2025

View reviewed changes

Update tests

f84346f

halleysfifthinc force-pushed the halfedge-topo-constructor branch from 8f116fa to f84346f Compare May 14, 2025 18:23

halleysfifthinc commented May 14, 2025

View reviewed changes

test/topologies.jl Show resolved Hide resolved

halleysfifthinc added 2 commits May 16, 2025 14:22

Rename REV_DIR => isreversed

d6bc0a0

Remove _ in NULL_EDGE name

608d7eb

halleysfifthinc mentioned this pull request May 16, 2025

Refactor adjsort into adjsortperm #1183

Open

Refactor for improved readability

dab47eb

juliohm reviewed May 20, 2025

View reviewed changes

src/topologies/halfedge.jl Show resolved Hide resolved

juliohm reviewed May 20, 2025

View reviewed changes

src/topologies/halfedge.jl Show resolved Hide resolved

halleysfifthinc commented May 20, 2025

View reviewed changes

src/topologies/halfedge.jl Outdated Show resolved Hide resolved

halleysfifthinc added 4 commits May 20, 2025 10:04

Avoid JuliaLang/julia#58470

7f0dab5

Directly set isreversed[other]

3a16da5

Add more details and improve phrasing for inconsistent edge error

c55d2fb

Fix formatting

c8b350d

Add additional comment

3c28952

juliohm reviewed May 20, 2025

View reviewed changes

src/topologies/halfedge.jl Outdated Show resolved Hide resolved

juliohm reviewed May 20, 2025

View reviewed changes

test/topologies.jl Outdated Show resolved Hide resolved

Update test/topologies.jl

b056e54

halleysfifthinc and others added 2 commits May 20, 2025 11:43

Unify reversed consistency branches

6d4b09c

Rename utility functions for improved readability

8ff06b6

juliohm approved these changes May 20, 2025

View reviewed changes

juliohm requested review from JoshuaLampert and Copilot May 20, 2025 19:43

Copilot AI reviewed May 20, 2025

View reviewed changes

src/topologies/halfedge.jl Show resolved Hide resolved

src/topologies/halfedge.jl Show resolved Hide resolved

src/topologies/halfedge.jl Show resolved Hide resolved

JoshuaLampert reviewed May 20, 2025

View reviewed changes

src/topologies/halfedge.jl Show resolved Hide resolved

juliohm merged commit 6f1cd35 into JuliaGeometry:master May 20, 2025
27 of 29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `HalfEdgeTopology(::Vector{<:Connectivity})` correctness #1194

Improve `HalfEdgeTopology(::Vector{<:Connectivity})` correctness #1194

halleysfifthinc commented May 13, 2025

juliohm left a comment

codecov bot commented May 14, 2025 •

edited

Loading

halleysfifthinc commented May 14, 2025

juliohm commented May 19, 2025

juliohm commented May 20, 2025

halleysfifthinc commented May 20, 2025

juliohm commented May 20, 2025

halleysfifthinc commented May 20, 2025

juliohm commented May 20, 2025

juliohm commented May 20, 2025

juliohm commented May 20, 2025

Copilot AI left a comment

juliohm commented May 20, 2025

juliohm commented May 20, 2025

JoshuaLampert left a comment

juliohm commented May 20, 2025

juliohm commented May 20, 2025

halleysfifthinc commented May 20, 2025

juliohm commented May 20, 2025 via email

JoshuaLampert commented May 20, 2025

Improve HalfEdgeTopology(::Vector{<:Connectivity}) correctness #1194

Improve HalfEdgeTopology(::Vector{<:Connectivity}) correctness #1194

Conversation

halleysfifthinc commented May 13, 2025

Footnotes

juliohm left a comment

Choose a reason for hiding this comment

codecov bot commented May 14, 2025 • edited Loading

Codecov Report

halleysfifthinc commented May 14, 2025

juliohm commented May 19, 2025

juliohm commented May 20, 2025

halleysfifthinc commented May 20, 2025

juliohm commented May 20, 2025

halleysfifthinc commented May 20, 2025

juliohm commented May 20, 2025

juliohm commented May 20, 2025

juliohm commented May 20, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

juliohm commented May 20, 2025

juliohm commented May 20, 2025

JoshuaLampert left a comment

Choose a reason for hiding this comment

juliohm commented May 20, 2025

juliohm commented May 20, 2025

halleysfifthinc commented May 20, 2025

juliohm commented May 20, 2025 via email

JoshuaLampert commented May 20, 2025

Improve `HalfEdgeTopology(::Vector{<:Connectivity})` correctness #1194

Improve `HalfEdgeTopology(::Vector{<:Connectivity})` correctness #1194

codecov bot commented May 14, 2025 •

edited

Loading